Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [#21810] Allow default pasting behaviour in FontSizePicker #21812

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

kirilzh
Copy link
Contributor

@kirilzh kirilzh commented Apr 23, 2020

Description

Fixes: #21810
Default paste behavior is not allowed for input fields.

Types of changes

Bug fix, non-breaking change.
Handling the case when you want to paste in an input field.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@@ -11,6 +11,10 @@ import { compose } from '@wordpress/compose';
*/
import { getPasteEventData } from '../../utils/get-paste-event-data';

function isNativePasteEvent( event ) {
return event.type === 'paste' && event.target.tagName === 'INPUT';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right check here, won't we have the same issues for contenteditables, textareas....
I feel the intent here is more:

  • trigger the native paste behavior if the target is inside the editor canvas

I believe the main issue here is that there's React event bubbling happening instead of native DOM event bubbling triggering the onPaste handler when it shouldn't.

One option to solve this is to avoid the React handlers or to check the event.target inside the handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it was more of an isolated case hence treating the symptoms rather than the cause. Will look into your suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR.

__experimentalCanUserUseUnfilteredHTML: canUserUseUnfilteredHTML,
} = getSettings();

const handler = ( event ) => {
Copy link
Contributor

@youknowriad youknowriad Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one small subtle difference that potentially can have performance impact. Since this is an inline function, potentially, this could rerender children often. I think it's good to start like you did (without any memoization) but we just need to check that the component is not being rerendered on each "store change".

For instance you can add a "console log" inside the component and type on a paragraph and see if the logging is repeated or not as you type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While typing console.log is not executed. When copying/pasting there are two statements in console.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine :) Thanks for the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting in FontSizePicker replaces the block itself
2 participants